Skip to content

Conversation

@MarekPieta
Copy link
Contributor

@MarekPieta MarekPieta commented Oct 14, 2024

Change cherry-picks commit that allows to workaround MPSL flash sync timeout issue.

Jira: NCSDK-29354
Jira: DRGN-23363

@MarekPieta MarekPieta requested review from a team as code owners October 14, 2024 14:12
@github-actions github-actions bot added manifest changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. labels Oct 14, 2024
@MarekPieta MarekPieta removed the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Oct 14, 2024
@NordicBuilder
Copy link
Contributor

NordicBuilder commented Oct 14, 2024

The following west manifest projects have been modified in this Pull Request:

Name Old Revision New Revision Diff
zephyr nrfconnect/sdk-zephyr@8411e6a nrfconnect/sdk-zephyr#2107 nrfconnect/sdk-zephyr#2107/files

Note: This message is automatically posted and updated by the Manifest GitHub Action.

@NordicBuilder
Copy link
Contributor

NordicBuilder commented Oct 14, 2024

CI Information

To view the history of this post, clich the 'edited' button above
Build number: 21

Inputs:

Sources:

sdk-nrf: PR head: 4244811b41274afc73ad612d72210c138f03dc73
zephyr: PR head: b642302d74f273b2aca949bc7df3209e95711321

more details

sdk-nrf:

PR head: 4244811b41274afc73ad612d72210c138f03dc73
merge base: 6d40ee41124cec57a9d9e6e02c51526d99ddbd28
target head (main): 6d40ee41124cec57a9d9e6e02c51526d99ddbd28
Diff

zephyr:

PR head: b642302d74f273b2aca949bc7df3209e95711321
merge base: 8411e6a346838a2d0980b28e9add8b29a06d711f
target head (main): 6ccfcdc1051d9d405f694882beb4f306ae518b5c
Diff

Github labels

Enabled Name Description
ci-disabled Disable the ci execution
ci-all-test Run all of ci, no test spec filtering will be done
ci-force-downstream Force execution of downstream even if twister fails
ci-run-twister Force run twister
ci-run-zephyr-twister Force run zephyr twister
List of changed files detected by CI (7)
west.yml
zephyr
│  ├── include
│  │  ├── zephyr
│  │  │  ├── fs
│  │  │  │  │ zms.h
│  ├── subsys
│  │  ├── bluetooth
│  │  │  ├── host
│  │  │  │  ├── Kconfig
│  │  │  │  ├── conn.c
│  │  │  │  ├── conn_internal.h
│  │  │  │  │ hci_core.c
│  │  ├── fs
│  │  │  ├── zms
│  │  │  │  │ zms.c

Outputs:

Toolchain

Version: 3dd8985b56
Build docker image: docker-dtr.nordicsemi.no/sw-production/ncs-build:3dd8985b56_912848a074

Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped; ⚠️ Quarantine

  • ◻️ Toolchain - Skipped: existing toolchain is used
  • ✅ Build twister
    • sdk-nrf test count: 886
    • sdk-zephyr test count: 6570
  • ✅ Integration tests
    • ✅ test-sdk-audio
    • ✅ test_ble_nrf_config
    • ✅ test-fw-nrfconnect-ble_samples
    • ✅ test-fw-nrfconnect-chip
    • ✅ test-fw-nrfconnect-nfc
    • ✅ test-fw-nrfconnect-nrf-iot_thingy91
    • ✅ test-sdk-find-my
    • ✅ test-sdk-sidewalk
    • ✅ test-low-level
    • ✅ test-sdk-dfu
Disabled integration tests
    • desktop52_verification
    • doc-internal
    • test-fw-nrfconnect-apps
    • test-fw-nrfconnect-ble_mesh
    • test-fw-nrfconnect-boot
    • test-fw-nrfconnect-fem
    • test-fw-nrfconnect-nrf-iot_cloud
    • test-fw-nrfconnect-nrf-iot_lwm2m
    • test-fw-nrfconnect-nrf-iot_mosh
    • test-fw-nrfconnect-nrf-iot_nrf_provisioning
    • test-fw-nrfconnect-nrf-iot_positioning
    • test-fw-nrfconnect-nrf-iot_samples
    • test-fw-nrfconnect-nrf-iot_serial_lte_modem
    • test-fw-nrfconnect-nrf-iot_zephyr_lwm2m
    • test-fw-nrfconnect-nrf_crypto
    • test-fw-nrfconnect-proprietary_esb
    • test-fw-nrfconnect-ps
    • test-fw-nrfconnect-rpc
    • test-fw-nrfconnect-rs
    • test-fw-nrfconnect-tfm
    • test-fw-nrfconnect-thread
    • test-fw-nrfconnect-zigbee
    • test-sdk-mcuboot
    • test-sdk-pmic-samples
    • test-sdk-wifi
    • test-secdom-samples-public

Note: This message is automatically posted and updated by the CI

@NordicBuilder
Copy link
Contributor

You can find the documentation preview for this PR at this link. It will be updated about 10 minutes after the documentation build succeeds.

Note: This comment is automatically posted by the Documentation Publishing GitHub Action.

@MarekPieta MarekPieta requested review from a team October 14, 2024 14:30
Copy link
Contributor

@rugeGerritsen rugeGerritsen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest to split this PR into two parts:

  1. Bring in the changes from zephyr
  2. Start using it

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it is a good idea to add an imply here for the SoftDevice Controller. We should rather enable it by default. That being said, we shouldn't enable experimental symbols for default builds

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The symbol was marked as experimental, because it's a temporary solution and we want to eventually drop it without following deprecation policies. From my observations, enabling the option might improve stability and prevent flash operation failures.

If the solution is not accepted NCS-wide, we could enable it e.g. only for applications maintained by us. Still, looking at the possible consequences of the fixed flash issue, I think that it may be reasonable to enable it by default if MPSL flash sync is used together with Bluetooth.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest I prefer imply over overriding Kconfig default here. While using imply, we do not need to worry about Kconfig dependencies. Overriding defaults could actually result in enabling a symbol even though dependencies of the symbol are not met (we would need to maintain the same set of dependencies in place where we override default to ensure consistent behavior).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could introduce an artificial symbol that would be enabled by default if both SoftDevice and MPSL flash sync are enabled? The symbol could imply the workaround.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you doing blocking operations on the system workqueue? It is very much discouraged to block a shared resource.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should look into making the flash API async

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, a lot of applications use system workqueue for flash operations (delegating settings store to system workq was quite common practice). Not sure if we can prevent users from doing it (we would need to e.g. add an execution context assertion to validate the requirement properly).

@MarekPieta MarekPieta added this to the 2.8.0 milestone Oct 15, 2024
@MarekPieta MarekPieta force-pushed the mpsl_timeout_fix_cherrypick branch from 86c2468 to 91461dc Compare October 15, 2024 12:12
@github-actions github-actions bot added the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Oct 15, 2024
@MarekPieta
Copy link
Contributor Author

MarekPieta commented Oct 16, 2024

After discussion with codeowners, we decided not to enable the workaround by default (CONFIG_BT_CONN_TX_NOTIFY_WQ is experimental).
We will introduce a known issue with link to the CONFIG_BT_CONN_TX_NOTIFY_WQ as a workaround.

btw. CI tests are green even with CONFIG_BT_CONN_TX_NOTIFY_WQ enabled by default.

@MarekPieta MarekPieta force-pushed the mpsl_timeout_fix_cherrypick branch from 91461dc to 848f3cc Compare October 16, 2024 09:26
@MarekPieta
Copy link
Contributor Author

Dropped commit enabling the workaround by default and rebased onto main.
We will open separate PRs to enable the workaround in scope of our applications.

@MarekPieta MarekPieta removed the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Oct 16, 2024
@MarekPieta MarekPieta force-pushed the mpsl_timeout_fix_cherrypick branch from 848f3cc to a8a9e01 Compare October 16, 2024 12:22
@github-actions github-actions bot added the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Oct 16, 2024
@MarekPieta MarekPieta force-pushed the mpsl_timeout_fix_cherrypick branch from a8a9e01 to 3904537 Compare October 16, 2024 12:41
@github-actions github-actions bot added the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Oct 16, 2024
@MarekPieta MarekPieta removed the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Oct 16, 2024
@MarekPieta MarekPieta force-pushed the mpsl_timeout_fix_cherrypick branch from 3904537 to d992a5c Compare October 16, 2024 13:10
@github-actions github-actions bot added the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Oct 16, 2024
@MarekPieta MarekPieta removed the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Oct 16, 2024
@MarekPieta MarekPieta force-pushed the mpsl_timeout_fix_cherrypick branch from d992a5c to 0782b51 Compare October 16, 2024 13:22
@github-actions github-actions bot added the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Oct 16, 2024
@MarekPieta MarekPieta removed the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Oct 16, 2024
@MarekPieta MarekPieta force-pushed the mpsl_timeout_fix_cherrypick branch from 0782b51 to 008e35e Compare October 17, 2024 06:00
@github-actions github-actions bot added the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Oct 17, 2024
@MarekPieta MarekPieta force-pushed the mpsl_timeout_fix_cherrypick branch from 008e35e to 0cecaa5 Compare October 17, 2024 10:04
@MarekPieta MarekPieta removed the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Oct 17, 2024
@MarekPieta MarekPieta force-pushed the mpsl_timeout_fix_cherrypick branch from 0cecaa5 to f2c0eb3 Compare October 17, 2024 12:26
@github-actions github-actions bot added the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Oct 17, 2024
@MarekPieta MarekPieta force-pushed the mpsl_timeout_fix_cherrypick branch 4 times, most recently from 959444e to 8875bcb Compare October 18, 2024 10:34
@thst-nordic
Copy link
Contributor

CI/Jenkins/toolchain Failing after 5m — building toolchain
rebase will fix this

@MarekPieta MarekPieta force-pushed the mpsl_timeout_fix_cherrypick branch from 8875bcb to 3cad513 Compare October 18, 2024 11:38
@MarekPieta
Copy link
Contributor Author

CI/Jenkins/toolchain Failing after 5m — building toolchain rebase will fix this

Rebased

@MarekPieta MarekPieta force-pushed the mpsl_timeout_fix_cherrypick branch 2 times, most recently from 102c4f5 to 053a2d6 Compare October 21, 2024 08:53
Change cherry-picks commit that allows to workaround MPSL flash sync
timeout issue.

Jira: NCSDK-29354
Jira: DRGN-23363

Signed-off-by: Marek Pieta <[email protected]>
@MarekPieta MarekPieta force-pushed the mpsl_timeout_fix_cherrypick branch from 053a2d6 to 4244811 Compare October 21, 2024 08:54
@MarekPieta MarekPieta removed the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Oct 21, 2024
@rlubos
Copy link
Contributor

rlubos commented Oct 21, 2024

Closing, will udpate SHA in other PR.

@rlubos rlubos closed this Oct 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants